[feat](inverted-index) Add IK tokenizers to custom analyzers - #67918
Open
airborne12 wants to merge 53 commits into
Open
airborne12 wants to merge 53 commits into
airborne12 wants to merge 53 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of exact head 1ecadcaebb31e4bdfc8139194e9434eb044cdea1. Requesting changes for 1 P0, 6 P1, and 1 P2 findings.
Critical checkpoint conclusions
- Goal and proof: The goal is to expose
ik_smartandik_max_wordas custom-analyzer tokenizers. The lowercase scalar happy path is wired, but the P0 reset regression and the policy, offset, and phrase failures mean the implementation does not yet satisfy the supported public paths. The added tests prove selected terms only. - Scope/focus: The production change is small and focused, but its integration surface is broader than the tests cover. No user-specific focus was provided, so the complete PR was reviewed.
- Concurrency: Factory and dictionary publication use
call_once; all production IK initializers use the same dictionary path, per-tokenizer mode remains local, and loaded tries are read-only. No new race, lock-order, deadlock, or heavy-under-lock issue was found. - Lifecycle/static initialization: Dictionary initialization is safe, but tokenizer reset lifecycle is not: CLucene's mandatory reset-before-consume crashes legacy IK indexing and destructively empties custom ARRAY token streams. Reader ownership and retained token storage are otherwise sound on the traced paths.
- Configuration: No new Doris runtime configuration item is added. Existing dictionary configuration remains restart-only and is used consistently.
- Compatibility: No wire/storage-format symbol is added, and the documented BE-then-FE rolling sequence covers factory availability. However, previously legal persisted policies named
ik_smart/ik_max_wordnow collide, with FE and BE choosing different meanings. - Parallel paths: Scalar custom reuse works, but legacy and ARRAY CLucene paths fail reset; both legacy CLucene and SNII phrase readers mishandle same-position pinyin alternatives, while the query-v2 CLucene path groups them.
- Conditional logic: No additional unsupported special condition was found. The missing FE validator cases and inconsistent normalization are called out inline.
- Test coverage: Missing cases include legacy IndexWriter, custom ARRAY, named TOKENIZER policies, mixed-case/noncanonical references, replay collisions, exact offsets, multibyte refill boundaries, and
MATCH_PHRASEacross storage implementations. - Test results: BE/FE tests and a regression suite were added, but their assertions do not cover the failing paths; the regression also violates required golden-output/table-name conventions. Per the review contract, I did not run builds or tests, so all conclusions are static and author/CI claims are not independent execution evidence.
- Observability: No new metrics or logging are required for this registry exposure; existing exception boundaries are adequate once the correctness defects are fixed. The ARRAY failure is silent data omission and must be corrected rather than observed around.
- Persistence/transactions: No transaction state machine changes. Existing policy journaling/replay is structurally used, but replayed-name collision semantics are incompatible as noted inline.
- Data writes: Index construction is materially affected: existing legacy IK writes can crash and custom IK ARRAY values can contribute no terms. No separate rowset/version/MoW issue was introduced.
- FE-BE variables: No new Thrift field is needed, but existing string-valued policy transport preserves a spelling that FE accepts and BE's exact factory lookup rejects.
- Performance: No new asymptotic hot-path or material memory-accounting issue was substantiated; repeated dictionary initialization is only a completed
call_oncecheck. - Other issues: After two full rounds, including separate lifecycle/phrase and dictionary/policy risk passes, all reviewers returned
NO_NEW_VALUABLE_FINDINGS. One char-filter candidate was disproven by concrete overrides and is intentionally not submitted.
Please address the eight inline findings before merge.
Contributor
TPC-H: Total hot run time: 16654 ms |
Contributor
TPC-DS: Total hot run time: 81543 ms |
Contributor
ClickBench: Total hot run time: 14.57 s |
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: The initial IK custom-analyzer integration did not fully preserve tokenizer reset semantics, document-relative UTF-8 offsets, case-insensitive built-in resolution, or upgrade compatibility when an existing named policy collides with a new built-in name. Its regression coverage also lacked generated golden output. Preserve the pending-reader reset contract, advance refill offsets by consumed bytes, rebase pinyin candidates to source-token byte spans, canonicalize built-in names while keeping named policies authoritative, accept IK tokenizer policy types in FE validation, and add focused unit and regression coverage. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test - Behavior changed: - [x] No. This corrects edge cases in the feature introduced by the preceding commit. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. Validation: - `./build.sh --be -j48` (ASAN) - `./build.sh --fe` - `./run-be-ut.sh --run --filter='IKTokenizerTest.*' -j48` (21 tests passed) - `./run-be-ut.sh --run --filter='AnalysisFactoryMgrTest.*:IKTokenizerTest.*:PinyinFilterTest.*:IndexPolicyMgrTest.*' -j48` (80 passed; one pre-existing order-dependent dictionary-initialization test failed, then passed in the isolated IK run) - `./run-fe-ut.sh --run --filter=org.apache.doris.analysis.invertedindex.AnalyzerIdentityBuilderTest` (6 tests passed) - `./run-fe-ut.sh --run --filter=org.apache.doris.indexpolicy.PolicyValidatorTests` (20 tests passed) - `./run-regression-test.sh --run -s test_ik_custom_analyzer -d inverted_index_p0/analyzer` (generated golden and clean comparison passed) - clang-tidy on all changed C++ lines with the ASAN compilation database; one test translation unit is blocked by the pre-existing unmatched `NOLINTEND` in `be/src/core/types.h` - C++ format, Checkstyle, build-hygiene, and glibc compatibility checks The Chinese literals in the tests and regression golden are tokenizer input and expected token data, not code descriptions.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16855 ms |
Contributor
TPC-DS: Total hot run time: 82284 ms |
Contributor
ClickBench: Total hot run time: 14.94 s |
Contributor
There was a problem hiding this comment.
Request changes — static review completed and converged at exact head 33dd82008708152608064761581e49b9d1475744 against base 2573820600eb178150a35710a94fd58b478df96d.
Five distinct P1 findings are attached inline: aggregate BE test-order dependence, locale-dependent IK name normalization, loss of source-byte boundaries after fullwidth normalization, non-canonical built-in filter identities, and unequal identities for direct versus type-only named IK tokenizers.
Critical checkpoint conclusions:
- Goal and proof: the patch registers both IK modes and adds FE, BE, and regression coverage, but the goal is not safely complete while the combined BE test binary is order-dependent and the offset/identity cases above remain uncovered.
- Scope: the production change is focused on analyzer registration, reset/offset propagation, policy validation, and identity. All 19 changed files and their upstream/downstream paths were reviewed; no additional user focus was provided.
- Concurrency: factory registration and dictionary initialization use process-wide
call_once; FE policy maps remain lock-protected. No runtime race, lock-order, or deadlock issue was found. The singleton lifecycle does create the accepted cross-test failure. - Lifecycle: fresh/reusable custom streams, legacy raw-reader streams, scalar/ARRAY indexing, reader ownership, and repeated reset were traced; the current reset fix preserves ownership and tokenizes once per reader. Global dictionary test state is not isolated.
- Configuration: no new dynamic or persisted configuration key is introduced; both factories consume the existing dictionary path and keep smart/max-word mode per stream. No runtime configuration propagation gap was found.
- Compatibility: existing named-policy precedence is aligned across FE replay/identity and BE resolution, and unsupported mixed-version direct use fails explicitly rather than silently reinterpreting metadata. Locale normalization and semantic identity consistency still need the inline fixes.
- Parallel paths and conditions: tokenizer, token-filter, and char-filter resolution plus both CREATE TABLE and CREATE INDEX identity consumers were checked. The filter and named-wrapper branches are the missing parallel cases reported inline.
- Tests and results: regression ordering/goldens are structurally deterministic, but negative Unicode/locale/identity coverage is missing. Per review-runner contract I ran no builds or tests. Author-reported validation includes a combined relevant BE run failure followed by an isolated IK pass; live CI currently has formatter, Checkstyle, license, secrets, Cloud UT, and performance passing while BE UT, FE UT, and compile remain pending.
- Observability: existing exception and logging paths are adequate for this local analyzer feature; no new metric is warranted.
- Persistence and transactions: policy image/replay, name-map rebuilding, propagation, and drop/reference paths were traced. No EditLog payload, transaction protocol, or storage-format change is introduced.
- Data writes and crash behavior: index-writer scalar/ARRAY paths were checked; no new transactional atomicity or crash-leak issue was found beyond incorrect offsets and duplicate-index admission.
- FE/BE variables: no new thrift field or scattered session variable is introduced; existing policy properties carry the component names.
- Performance and memory: eager IK buffering is pre-existing; retaining two offsets per token adds bounded metadata and no new hot-loop or tracked-allocation regression was substantiated.
- Other correctness: error propagation and position increments are consistent. All prior threads were treated as hard duplicate fences, every candidate was resolved, and the third round returned
NO_NEW_VALUABLE_FINDINGSfrom all reviewers.
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: The IK analyzer integration could initialize global dictionaries from a factory-resolution test, depend on the JVM default locale, produce distinct identities for equivalent built-in components, and lose original byte boundaries when IK regularization changes UTF-8 width. Keep the factory-resolution test side-effect free, normalize names with Locale.ROOT, canonicalize all built-in component identities and type-only wrappers, and propagate source rune boundaries through token filters so pinyin offsets remain relative to the original document. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test - Behavior changed: - [x] No. This corrects edge cases in the feature introduced by the preceding commits. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. Validation: - ./build.sh --be -j8 (ASAN) - ./build.sh --fe -j8 - IKTokenizerTest and PinyinFilterTest (62 tests passed) - AnalysisFactoryMgrTest, IKTokenizerTest, PinyinFilterTest, and IndexPolicyMgrTest (82 tests passed) - AnalyzerIdentityBuilderTest and PolicyValidatorTests (29 tests passed) - clang-format 16 and Checkstyle passed - clang-tidy found no warnings on analyzable changed lines; one deletion-only test translation unit remains blocked by a pre-existing unmatched NOLINTEND in be/src/core/types.h The fullwidth literal in the test is tokenizer input data, not a code description.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review complete at head c5976f7cff0c8aae1f3dc174a377d1446b70c315 after two convergence rounds. Requesting changes for one distinct P1 issue described inline.
Critical checkpoint conclusions:
- Goal and test approach: The PR wires
ik_smartandik_max_wordthrough FE policy validation, BE factory construction, TOKENIZE/query, index writing, and regression coverage. The implementation goal is coherent, but semantic duplicate detection is incomplete across the legacy and custom IK entry points. - Scope and focus: Reviewed all 22 authoritative changed paths and their confirmed callers/consumers. The user supplied no additional focus, so the full PR remained in scope.
- Concurrency: FE policy-map locking, factory registration, and the process-wide IK dictionary initialization were traced; no distinct new runtime race or lock-order defect was found.
- Lifecycle and static initialization: Legacy/custom raw and shared readers, reusable scalar streams, ARRAY streams, reset/error recovery, dictionary lifetime, and filter span lifetime were checked. Residual test-order concerns are already covered by an existing thread and were not duplicated.
- Configuration: No new dynamic-config contract is introduced. Both IK modes use the established dictionary path; configuration normalization issues substantially covered by existing threads were fenced.
- Compatibility and rolling upgrade: No new persisted or wire schema is added. Policy replay/publication and existing-policy-first name collisions were checked; normal BE-first rollout is required for the new factories, with no separate compatibility defect found.
- Parallel and special-case paths: Checked smart, implicit-smart, and max-word modes; direct and named/type-only references; CREATE TABLE and ALTER/CREATE INDEX; scalar and ARRAY writes; TOKENIZE, MATCH, and query analysis. The cross-family duplicate gap affects both DDL paths.
- Conditionals, comments, and similar implementations: Compared legacy
IKAnalyzerconstruction withIKTokenizerFactory, reviewed normalization precedence and related tokenizer/filter factories, and traced source-offset/refill/truncation branches. No additional distinct issue survived the duplicate fences. - Tests: Changed BE unit, FE unit, and regression tests cover registration, validation, reset/reuse, refills, offsets, pinyin composition, both IK modes, TOKENIZE, and MATCH. Cross-family duplicate tests are missing and are requested with the inline finding. This reviewer ran no builds or tests because the review contract prohibited execution. The author reports ASAN builds, 82 combined BE tests, 29 FE tests, and the regression case; at submission time BE UT, FE UT, compile, performance, and review checks were still pending in CI.
- Outputs: The regression golden is ordered/deterministic for the asserted queries and follows cleanup-before-use conventions. No distinct output-contract issue was found.
- Observability and errors: Existing exceptions/status propagation and relevant logging remain in place; no new silent production failure or observability gap was found outside the identity bug.
- Persistence and transactions: Index-policy journal/replay/image reconstruction and FE-to-BE publication were traced. No transaction-state, metadata-format, or replay-order defect was found.
- Writes, atomicity, and crash behavior: Scalar/ARRAY index-writer consumption and reset ordering were reviewed; no new atomicity, partial-write, or crash-recovery issue was found.
- FE/BE variables and all consumers: No Thrift field is added. FE names and BE factories align for valid direct/named policies, but FE's analyzer identity does not unify the equivalent legacy IK representation, which is the blocking finding.
- Performance: Source-byte mapping is allocated only for width-changing normalization and the identity work remains DDL-side; no material new performance issue was substantiated.
- Other: Security review was not requested or implicated. Existing inline issues and replies were treated as hard duplicate fences and were not reposted.
Review status: complete static review; one new blocking finding; exact base/head verified before submission.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: Legacy IK parser indexes and equivalent custom analyzers used different semantic identities, allowing duplicate inverted indexes on one column. Canonicalize unfiltered legacy smart and max-word modes to the corresponding custom analyzer identities while preserving distinct identities for filtered legacy analyzers. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] Yes. Equivalent legacy and custom IK indexes are now rejected as duplicates. - Does this need documentation? - [x] No.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16675 ms |
Contributor
TPC-DS: Total hot run time: 81320 ms |
Contributor
ClickBench: Total hot run time: 14.6 s |
Contributor
There was a problem hiding this comment.
Static review complete at exact head 0e66fd9522552abd9efa2bb685db6970859b19d1 against base 2573820600eb178150a35710a94fd58b478df96d after three bounded convergence rounds. Requesting changes for three P1 findings and one P2 finding attached inline.
Critical checkpoint conclusions
- Goal and proof: The PR exposes
ik_smartandik_max_wordto custom analyzers and covers the ordinary factory, reset, direct-offset, TOKENIZE, MATCH, CREATE, and ALTER paths. The happy path is coherent, but lazy error propagation, semantic identity, transforming-filter offsets, and unconditional resource costs keep the feature from being safely complete. - Scope and focus: The production change is generally focused on factory registration, tokenizer lifecycle/offsets, policy validation, and analyzer identity. All 22 authoritative changed files and their relevant callers/consumers were reviewed. No additional user focus was supplied, so the full PR remained in scope.
- Concurrency: Factory publication and dictionary initialization use the existing process-wide
call_oncemechanisms; FE policy-map access remains under the existing reentrant lock discipline. No new thread, race, heavy-under-lock operation, inconsistent lock order, or deadlock issue was found. - Lifecycle/static initialization: Raw and shared readers, eager and reusable streams, reset-before-consume, EOF behavior, and source-map lifetime were traced. Reset and ownership are sound on supported production paths. The dictionary singleton itself is stable, but its new lazy creation point exposes the uncaught error boundary reported inline; no cross-TU static-order hazard was found.
- Configuration: No new Doris runtime configuration key is added. Both modes use the established IK dictionary path and per-stream mode. No dynamic-configuration propagation gap was found.
- Compatibility: No wire or storage-format field changes. Direct/case-variant factories align across FE and BE, and supported BE-before-FE rolling order avoids an old-BE factory gap. Replayed pre-upgrade tokenizer-name collisions are nevertheless misused by the new synthetic legacy identity, as reported inline.
- Parallel paths: CREATE and ALTER share the faulty identity. TOKENIZE, row-fallback MATCH, and analyzed SEARCH all reach lazy tokenizer creation after their effective CLucene conversion boundary. Scalar/ARRAY writer and VARIANT metadata paths were separately traced; legal FE metadata does not route a custom analyzer into ARRAY ingestion, so that unsupported path is intentionally excluded from the finding.
- Special conditions: The conditional reset is safe in the traced caller sequences. The legacy-identity guard omits behavior-affecting state, and term-text-only source-map forwarding is not compositional through a supported transforming filter; both are concrete inline findings.
- Test coverage: Changed tests cover both modes, default legacy/custom duplicate detection in CREATE and ALTER, raw/shared reset contracts, synthetic IndexWriter ARRAY consumption, refill/direct full-width offsets, and deterministic TOKENIZE/MATCH results. Missing cases correspond to the four findings: first-use dictionary failure, legacy
lower_case=false, replay-plus-duplicate identity, transforming-filter source offsets, and resource behavior. The residual dictionary test-order concern is already covered by an existing thread and was not reposted. - Test results: The regression uses ordered/deterministic output, pre-test drops, hardcoded table names, and expected-error form. Per the review contract, I ran no build or test, so these conclusions are static. Author-reported ASAN builds, combined BE tests, FE tests, and regression execution were treated as author evidence, not independent verification.
- Observability: Existing dictionary logging is sufficient once failures are converted into Doris errors; no new metric is warranted. Letting a raw
CLuceneErrorescape is an error-contract defect, not an observability substitute. - Persistence and transactions: Policy image/replay/name-map rebuilding was traced. No EditLog payload, transaction state machine, failover, or metadata-format change is introduced; replayed-name state is relevant only to the identity finding.
- Data writes and crash behavior: Analyzer output affects inverted-index term generation, but no rowset/version/MoW, atomicity, partial-write, or crash-recovery defect was found. Direct offset/reset writer paths are sound; the supported transforming-filter chain still publishes incorrect source offsets.
- FE-BE variables: No new Thrift or session variable is added. Existing string-valued policy properties carry the tokenizer/filter names; ordinary resolution agrees, with the synthetic-identity exception reported inline.
- Performance and memory: The new source-map detector and enlarged eager token records affect every legacy/custom IK stream even when Pinyin is absent. This definite redundant scan and retained-memory growth is the P2 finding; no separate RTTI or memory-accounting finding was substantiated.
- Other issues: Existing rendered threads and raw comments were treated as hard duplicate fences. Every candidate was ledgered and adjudicated. The final round narrowed supporting evidence but did not add or remove a finding; the stable four-finding set is complete within the three-round bound. Security review was neither requested nor implicated.
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: Lazy IK dictionary failures could escape the custom analyzer exception boundary, legacy IK identities could collide with behaviorally different or shadowed custom tokenizers, and pinyin offsets lost source-byte provenance after word delimiter processing. Source offset maps were also built even when no downstream filter consumed them. Translate analyzer construction errors, keep legacy identity matching limited to equivalent built-in IK behavior, and make source-byte provenance opt-in and compositional through token filters. Preserve and rebase provenance for generated word delimiter parts and concatenations. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] Yes. IK custom analyzers now report Doris analyzer errors, keep distinct identities for non-equivalent legacy settings, and preserve exact source offsets through delimiter and pinyin filters without unconditional mapping work. - Does this need documentation? - [x] No.
Member
Author
|
run buildall |
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review result: no new non-duplicate findings on exact head 4ce410ca359c5e3e8842d5759dfe655881f9f503 (base 69b803b32a5ec914a4f4bfdaf45f6347c48435a0).
I reviewed the complete 95-file authoritative diff and converged after two independent review rounds. Plausible residuals were either already covered by the existing inline threads or disproved by the enclosing code (notably, VMatchPredicate construction is already inside VExpr::create_expr()'s Exception-to-Status boundary). I am not reposting those issues. Existing inline discussions remain the actionable review record; this summary does not clear or supersede them.
Checkpoint conclusions:
- Goal, correctness, and tests: the changes consistently extend custom IK analysis, analyzer identity/policy binding, provenance-aware offsets, reader selection, and exception conversion across the reviewed paths. The added FE/BE unit and regression coverage exercises DDL, replay/collision cases, analyzer equivalence, malformed UTF-8, offset mapping, writer/reader failures, and query selection. No additional correctness defect survived duplicate and reachability checks.
- Scope and focus: the broad FE/BE/test scope is cohesive around analyzer configuration and execution. No additional user-provided focus was supplied, and no extra issue was found for that focus.
- Concurrency: FE/BE policy registries, analyzer-provider/factory reuse, and selected-reader state were checked against their lock/ownership patterns. I found no new race, lock-ordering, or unsafe shared-state issue.
- Lifecycle and persistence: journal/image replay, exact and normalized name maps, drop/re-registration, analyzer stream reset/reuse, cached providers, and high-water cleanup were traced. Remaining legacy binding/migration concerns are already represented by existing threads.
- Configuration and compatibility: no new server configuration item is introduced. Existing dictionary/analyzer properties, canonical built-ins, exact-name precedence, legacy aliases, serialized metadata, and mixed FE/BE behavior were reviewed; no new non-duplicate compatibility finding emerged.
- Parallel paths: classic and SNII readers/writers, scalar and ARRAY writes, MATCH/slow fallback, direct SEARCH, scoring, typed reader selection, and phrase-capability gating were all checked, including lazy analyzer construction/tokenization failures.
- Test results and observability: expected outputs and negative cases were inspected statically. Error conversion and existing logging are adequate for the changed failure paths; I found no distinct metrics/logging gap requiring a review comment.
- Data safety, atomicity, and performance: this PR does not introduce a transaction or physical storage-format change. Policy-map updates, offset/provenance caps, malformed-input handling, reset/reuse, and memory high-water behavior were reviewed; no new atomicity, CPU, or memory issue was substantiated beyond existing threads.
Validation was static only, as required by the review environment. I did not build Doris or run tests.
Contributor
TPC-H: Total hot run time: 27867 ms |
Contributor
TPC-DS: Total hot run time: 152654 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
…zed reader Moving the support_phrase check into select_best_reader() made it run for every selected reader and for every caller. That broke two things the old preflight never touched: - an untokenized index (USING INVERTED without a parser) never declares support_phrase and answers MATCH_PHRASE by matching the whole value as one term; the check rejected it (inverted_index_p0.test_index_complex_match, inverted_index_p0.test_omit_norms); - direct SEARCH resolves its reader through select_best_reader() and has never enforced support_phrase; its regression suites run phrase clauses on parser indexes without it (search.test_search_function, search.test_search_dsl_syntax, search.test_search_multi_analyzer_lucene). The check now lives in read_from_index(), applies only when the selected reader is a FULLTEXT reader, and still checks the reader the analyzer selected rather than the first full-text candidate. SEARCH keeps its previous behaviour. Tests: PhraseSupportIsCheckedOnTheSelectedReader now drives read_from_index() in both index orders; PhraseQueriesStillRunOnAnUntokenizedIndex covers the untokenized case for MATCH_PHRASE, MATCH_PHRASE_PREFIX and MATCH_PHRASE_EDGE.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for one backward-compatibility regression in explicit IK index selection.
Critical checkpoints:
- Goal and correctness: the custom IK analyzer, exact policy binding, and configuration-aware reader selection are broadly coherent, but the FE now rejects an existing explicit query/index pairing before those mechanics can run.
- Lifecycle and concurrency: exact/normalized policy registration, locking, create/drop, journal replay, image rebuild, and dependency checks were traced without another unresolved issue.
- FE/BE compatibility and parallel paths: MATCH, direct/variant SEARCH, scoring, CLucene/SNII writers and readers, selected-reader phrase handling, and exception conversion were reviewed. Once FE selects an index, parser mode/lowercase/filter serialization agrees with the BE reader key; the accepted issue is the preceding FE compatibility gate.
- Memory, offsets, and performance: provenance through changed tokenizers/filters, clipping, malformed input, reset/reuse, and high-water cleanup were reviewed with no new non-duplicate finding.
- Tests: the added unit and regression coverage is extensive, but it locks in rejection of legacy parser=ik and lacks the single-existing-index upgrade case described inline. Per the review instructions, this was static analysis only; no builds or tests were run, so author/CI results were not independently reproduced.
- Scope and focus: all 95 authoritative changed paths were swept, existing inline threads were treated as duplicate fences, and the user supplied no additional focus beyond the full review.
Contributor
TPC-H: Total hot run time: 27566 ms |
Contributor
TPC-DS: Total hot run time: 152366 ms |
Contributor
ClickBench: Total hot run time: 23.68 s |
…cy IK index Explicit built-in IK selects an index by its effective configuration since 4501552, so a pre-upgrade table whose only IK index is parser=ik (ik_smart) no longer matched MATCH ... USING ANALYZER ik and the query failed with "No inverted index found for analyzer 'ik'". OlapTable.filterIndexesByAnalyzer() now falls back to the single index that carries the requested name when no index matches the configuration. The name check is InvertedIndexUtil.isAnalyzerNameMatched(), split out of isAnalyzerMatched(), which is unchanged. The predicate serializes that index's own parser_mode and lower_case, so BE binds its reader and tokenizes the query the same way. Two differently configured legacy indexes remain ambiguous. Tests: InvertedIndexPropertiesTest covers the single legacy index, legacy plus default, and the ambiguous pair; test_analyzer_identity_semantics adds a table whose only index is parser=ik and checks smart-mode results.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review result: request changes for one P1 FE/BE analyzer-identity mismatch.
A replayed legacy TOKEN_FILTER named LOWERCASE can make FE assign the built-in lowercase normalizer the identity of an ASCII-folding policy, even though BE executes the built-in lowercase pipeline. CREATE and ALTER can consequently reject two indexes whose runtime analyzers differ; the inline comment gives the concrete chain.
Critical checkpoint conclusions:
- Goal and proof: the PR broadly adds custom IK tokenizers and aligns replay binding, analyzer selection, semantic duplicate fences, failure conversion, and source-byte provenance. The remaining false-equivalence case prevents the duplicate-index goal from being complete. Existing tests are broad but do not cover a case-distinct legacy TOKEN_FILTER shadowing the built-in normalizer identity.
- Scope and clarity: all 96 authoritative changed paths were reconciled. The production and test changes are cohesive around analyzer policies, selection, execution, and provenance; no unrelated production change remained.
- Concurrency: FE policy maps retain read/write-lock protection, BE policy maps retain shared-mutex protection, and mutable analyzer/provenance state is instance-local. No lock-order, race, deadlock, or heavy-operation-under-lock defect was substantiated.
- Lifecycle, persistence, and compatibility: create, image/journal replay, policy reporting, drop/fallback restoration, provider binding, reader selection, reset/reuse, and the supported BE-first compatibility path were traced. MAIN-01 is the remaining replay-state mismatch; no other lifecycle, failover, or rolling-upgrade issue survived review.
- Configuration and protocol: no new dynamic server configuration or Thrift field is introduced. Analyzer/mode/lowercase/filter properties reach the relevant FE and BE paths consistently outside the inline identity bug.
- Parallel paths and error handling: classic and ARRAY writers, MATCH, direct SEARCH, Variant, scoring, CLucene/SNII readers, compaction, and phrase checks were compared. New IK failures and changed ICU/NGram failures reach Doris/CLucene-to-Status boundaries. A scalar Pinyin exception gap considered during review predates this PR and was not submitted as a PR finding.
- Data writes and atomicity: analysis failures abort document construction instead of publishing partial index state. No transaction, version, MoW, Cloud, crash-consistency, or nullable-column change is introduced here.
- Memory and performance: provenance maps and scratch buffers are reset/reused and oversized scratch is released. No distinct leak, unsafe lifetime, allocator issue, or material hot-path regression survived review.
- Tests and expected results: FE/BE unit and regression changes cover current binding, selection, malformed input, reset/reuse, and many semantic aliases. Missing blocking coverage is CREATE and ALTER with replayed
LOWERCASE(type=asciifolding), built-inLowerCase, and a named ASCII-folding normalizer. Checked-in expected outputs were inspected statically. - Observability and security: current analyzer statuses and policy logs are adequate for these paths; the change introduces no authentication, authorization, tenancy, or new outbound trust boundary.
- User focus: no additional focus was supplied.
Validation was static only against the authoritative merge-base-to-head diff and related call chains. I did not run builds or tests, and author/CI results were not independently revalidated.
Contributor
TPC-H: Total hot run time: 28457 ms |
Contributor
TPC-DS: Total hot run time: 153084 ms |
Contributor
ClickBench: Total hot run time: 24.08 s |
Member
Author
|
run external |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
run external |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
yiguolei
approved these changes
Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Custom inverted-index analyzers could not use IK smart or max-word tokenization with their configured character and token filters. This adds the
ik_smartandik_max_wordtokenizer factories and policy validation.Composed pipelines need source-byte provenance across normalization, splitting, concatenation, trimming, and reset/reuse. Opt-in offset tracking propagates through IK, keyword, standard, ICU normalization, word-delimiter, and pinyin processing. Separate rune end offsets preserve gaps removed by delimiter concatenation.
Review fixes preserve exact saved policy bindings across replay, lifecycle operations, MATCH serialization, provider dispatch, physical reader selection, and scoring-statistics collection. FE matches explicit built-in IK to its default max-word configuration. BE selection keys distinguish mode, lowercase configuration, and effective outer character filters without changing provider names, Thrift, or stored index formats. Encoded configurations cannot collide with arbitrary policy names.
For backend-first upgrades, BE now binds an old FE's normalized analyzer name to the canonical saved policy and accepts a lowercase metadata key only when it resolves to the same policy. Exact metadata remains preferred. The compatibility alias cannot select a differently bound policy or a built-in analyzer with different tokenization.
Named character-filter identities account for transformations absorbed by the resolved tokenizer while preserving meaningful ordered transformations. The two new global-policy suites run in
nonConcurrentinstead of contributing to concurrent P0 policy-quota exhaustion.Classic writer and query boundaries now convert lazy custom-analyzer failures to non-OK
Statusresults. This includes ARRAY token-stream creation beforeadd_document(), which clears the partial document and closes the writer on error instead of allowing a Doris exception to escape. FE identity construction also preserves case-folding absorption through the defaulticu_normalizernfkc_cffilter, preventing equivalent lower-then-fold and fold-only pipelines from receiving different identities.FE policy validation now checks an exact replayed policy before falling back to a normalized built-in analyzer or normalizer. This prevents a legacy wrong-type
IKorLOWERCASEpolicy from passing FE validation and failing later in BE.CREATE ... IF NOT EXISTSalso returns for an existing replayedik_smartorik_max_wordpolicy before applying the reserved built-in-name check, while fresh creations with those names remain rejected.Nested tokenizer, token-filter, and character-filter validation uses the same exact-first policy lookup as BE and enforces the expected component type. Replayed case-colliding policies can no longer bind a different saved component or the wrong filter family. When ICU token normalization changes text, its output carries a conservative source map to the original token span; unchanged tokens continue to delegate their precise upstream maps, and reset/reuse clears rewritten-map state.
The latest AI-review fixes reject duplicate non-IK analyzer selectors before full identity comparison, including the implicit default selector, so outer character-filter differences cannot create two indexes that the query syntax cannot distinguish. Basic, CharGroup, Empty, ICU, and NGram tokenizers now publish document-relative source boundaries for offset-aware Pinyin. ICU resolves monotonically increasing UTF-16 break boundaries with a constant-size UTF-8/UTF-16 cursor, transformations that change rune counts use a conservative whole-source span, and NGram byte offsets preserve full
UChar32values.The memory fixes remove full-input dense offset tables from ICU tokenization and normalization. The tokenizer preserves exact spans with its incremental cursor. The normalizer retains opt-in provenance, stores no provenance when unused, and represents a changed token with one compact conservative source span when enabled; Pinyin consumes that span without forcing dense maps.
The current review fixes prevent rune-expanding ASCII folding and lowercase filters from forwarding invalid exact provenance, avoid allocating Pinyin rune and source arrays in the default ignore-offset mode, canonicalize FE analyzer identities using effective component defaults, and convert lazy scoring-analyzer failures back into
Statusresults.The final CI follow-up aligns the duplicate-analyzer unit test with the intentional selector-based diagnostic introduced by those validation fixes.
Current candidate:
31450230c09d6ba10b6f0c2e8de70f3765948543.Release note
Add IK smart and max-word tokenizers to custom inverted-index analyzers. Correct composed-tokenizer source offsets, preserve custom-analyzer MATCH bindings during backend-first upgrades, select the matching physical IK configuration, and fix effective duplicate-index identities.
Check List (For Author)
add_document()exception boundary. The completeInvertedIndexWriterTestsuite passed 20/20.AnalyzerIdentityBuilderTesttests. After policy-validation repairs, it also passed the replay/image coverage and all 40PolicyValidatorTests;./build.sh --fecompleted successfully with zero Checkstyle violations.IndexPolicyMgrTest,PinyinFilterTest, andICUNormalizerFilterFactoryTest, including component type collisions and ICU-normalizer-to-pinyin source spans before and after reset.InvertedIndexPropertiesTest.AnalyzerIdentityBuilderTestandInvertedIndexPropertiesTest; the full 80-module Maven reactor and Checkstyle completed successfully.SchemaChangeHandlerTesttests and the complete 80-module Maven reactor after updating its stale duplicate-analyzer diagnostic expectation.test_ik_custom_analyzer,test_analyzer_identity_semantics, and existingtest_ik_analyzerpassed earlier against the task-owned native ASAN cluster. The targeted analyzer-identity regression passed 1/1; the Basic CREATE/ALTER cases added by the latest review fix were not run locally.bf127c2d9ca1cdbb70c604320120fea1f1e33002; exact merge-tree preflight against target snapshot3750a819a2af157ad539d947bc301ff0f806b928passed before publication.Component-level replay, serialization, dispatch, reader selection, scoring selection, and current-version end-to-end regression were validated. A full mixed-version cluster upgrade was not run. Existing IK internal lowercasing behavior is unchanged; the uppercase SQL probe is not treated as proof of a lowercase-disabled runtime fix.